feat: add config-driven release_level utility#220
Conversation
Add a generic `release_level` to `mozilla_taskgraph.util.attributes` so projects can determine whether a task is a "production" or "staging" release without hardcoding project-specific branch data in shared code. The set of release branches is project-specific, so it is sourced from a new `release-branches` mapping in the graph config rather than baked into mozilla-taskgraph. `release_level` reads the resolved value from the `release_branches` parameter (`True` means every branch of the project is a release branch, a list restricts releases to the named branches). It is read from params rather than the graph config so it is also available to consumers that only have a `Parameters` object, such as action tasks. A project turns this on by adding `release-branches` to its `config.yml` and calling `set_release_branches` from its decision-parameters function. Until a project does both, the `release_branches` parameter stays None and nothing uses the new utility, so adding this code has no effect on any existing project's builds.
|
I appreciate the full explanation of what this is doing, thank you! Is there some additional context here you can provide as to what is motivating this? (It's helpful to understand the context it will be used in when reviewing it.) |
| if params["level"] != "3": | ||
| return "staging" | ||
|
|
||
| branches = params.get("release_branches") |
There was a problem hiding this comment.
Is there a reason we want to route these through parameters? Parameters are typically for things that are associated with a specific event; these are clearly not. IMO it would be preferable to have callers pass along the graph config here (or simply the release-branches from the graph config) instead of adding this indirection.
There was a problem hiding this comment.
The reason is that there are callers that don't have graph_config available. For example the RUN_ON_PROJECT_ALIASES path.
https://searchfox.org/firefox-main/source/taskcluster/gecko_taskgraph/target_tasks.py#117
There was a problem hiding this comment.
I'm not seeing any consumers of release_level in target_tasks.py? (In any case, the target task handler functions do get passed the graph config if it is needed by anything they call, eg: https://searchfox.org/firefox-main/rev/bbdd22a8b58a05e5b269fe341b02ce11426c550f/taskcluster/gecko_taskgraph/target_tasks.py#475-477)
There was a problem hiding this comment.
match_run_on_projects is called in target_tasks.py, which in turn calls (RUN_ON_PROJECT_ALIASES[alias](params):
RUN_ON_PROJECT_ALIASES = {
# key is alias, value is lambda to test it against
"all": lambda params: True,
"integration": lambda params: (
params["project"] in INTEGRATION_PROJECTS or params["project"] == "toolchains"
),
"release": lambda params: (
release_level(params) == "production" or params["project"] == "toolchains"
),
There was a problem hiding this comment.
Ah, thank you - I missed that indirection. In any case, graph config is available to be fed through as mentioned, so there should be no need to add a new parameter here.
There was a problem hiding this comment.
Sorry, I double checked. You're right. The task handlers do have graph_config in scope, only the plumbing is not there. So that would need to be updated.
If we go down that path, then we can avoid setting release_branches in parameters here, and make release_level's signature as:
def release_level(graph_config, params)
I believe we still need params in order to access level and head_ref?
Does that make sense?
There was a problem hiding this comment.
Personally, I would pass the specific things that are needed from them instead of the entire data structures (it makes the signature more readable, and reduces the temptation to depend on more and more parts of those data structures), but I wouldn't r- this either way.
There was a problem hiding this comment.
Hey @bhearsum, I've just a pushed a new commit where I implemented you suggestions. Let me know what you think.
release_level now takes release_branches (directly, instead of the whole graph_config), however I decided to keep params in the function signature instead of specifying the other 3 pieces of data it requires (level, head_ref and project).
| if branches is True: | ||
| return "production" | ||
|
|
||
| m = re.match(r"refs/heads/(\S+)$", params["head_ref"]) |
There was a problem hiding this comment.
Is there a way to write this function such that we only have two returns? It would be easier to read if eg: we returned production under the right conditions, and then staging in any other case. Perhaps:
if level == "3":
if branches == True or <regex matches>:
return "production"
return "staging"
Instead of populating a `release_branches` decision parameter at decision time, pass the graph config's `release-branches` mapping to release_level directly. - `release_level` now takes `(release_branches, params)` - drop `MozillaParametersSchema`, `register_parameters` and `set_release_branches` - update tests accordingly
bhearsum
left a comment
There was a problem hiding this comment.
This is looking really good - much simpler and cleaner this time around!
Requesting changes for the bit about being stricter about the release_branches type, the other part is up to you.
| """ | ||
|
|
||
| if params["level"] == "3": | ||
| branches = (release_branches or {}).get(params["project"]) |
There was a problem hiding this comment.
Rather than allow release_branches to be something other than a dict, let's just assume it is, and require callers to make that happen. (And maybe add a type annotation to help any callers that run linters?)
There was a problem hiding this comment.
Yeah, that sounds good!
There was a problem hiding this comment.
Thanks again for the feedback. The latest commit should address these points.
| if params["level"] == "3": | ||
| branches = (release_branches or {}).get(params["project"]) | ||
| if branches is True or ( | ||
| branches |
There was a problem hiding this comment.
Totally optional, but you could break out this part into an intermediate variable to use in the if expression to avoid multiple lines for this.
There was a problem hiding this comment.
I've restructured the control flow a bit. Now I have 3 return statements, however I think that makes it easier to read and makes the intent and checks clear. Let me know what you think
- annotate both args as dicts and require release_branches to be a mapping
(drop the `or {}` fallback; callers pass {} when unconfigured)
- restructure the branch checks
bhearsum
left a comment
There was a problem hiding this comment.
I agree this is cleaner with the extra return!
| if branches is True: | ||
| return "production" | ||
|
|
||
| if isinstance(branches, list): |
There was a problem hiding this comment.
There's a possible edge case here of level == 3 and branches is not a list (eg: perhaps it's a string). That will end up falling back to "staging".
I think this is fine; just calling it out explicitly in case you want to address it.
There was a problem hiding this comment.
Thanks for that. I believe that's fine too. I think it is better to fallback to staging in the event that release_branches is "malformed" in config.yml, rather than the other way around. By "malformed" I mean not honoring the types defined in the schema MozillaGraphConfigSchema. Do you agree?
There was a problem hiding this comment.
Yeah, I think falling back in this way is fine. If it becomes a problem in the future we can always update the code.
feat: add config-driven release_level utility
Add a generic
release_leveltomozilla_taskgraph.util.attributesso projects can determine whether a task is a "production" or "staging" release without hardcoding project-specific branch data in shared code.The set of release branches is project-specific, so it is sourced from a new
release-branchesmapping in the graph config rather than baked into mozilla-taskgraph.release_levelreads the resolved value from therelease_branchesparameter (Truemeans every branch of the project is a release branch, a list restricts releases to the named branches). It is read from params rather than the graph config so it is also available to consumers that only have aParametersobject, such as action tasks.A project turns this on by adding
release-branchesto itsconfig.ymland callingset_release_branchesfrom its decision-parameters function. Until a project does both, therelease_branchesparameter stays None and nothing uses the new utility, so adding this code has no effect on any existing project's builds.AI disclosure: these changes have been assisted by AI.